Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for shadow DOM #604 #606

Merged
merged 8 commits into from
Jul 16, 2024
Merged

Conversation

dgr
Copy link
Contributor

@dgr dgr commented Jul 14, 2024

Please complete and include the following checklist:

  • [x ] I have read CONTRIBUTING and the Etaoin Developer Guide.

  • [x ] This PR corresponds to an issue that the Etaoin maintainers have agreed to address.

  • [x ] This PR contains test(s) to protect against future regressions

  • [x ] I have updated CHANGELOG.adoc with a description of the addressed issue.

@lread
Copy link
Collaborator

lread commented Jul 14, 2024

Nice work @dgr! Thanks for taking so much care!

Some thoughts/ideas/observations:

Maybe docstrings could link to Etaoin user guide which in turn could link to https://wpt.fyi/results/webdriver/tests/classic/find_element_from_shadow_root/find.py?label=experimental&label=master&aligned in a prominent adoc NOTE. This way users would get a link to your very nice overview of the shadow dom (I read, I learned!) AND a link to current implementation limitations. Whaddya think?

Test-doc is failing because code blocks will return different results each time they are run. For example, this gets converted into a test:

(e/get-element-shadow-root driver {:id "shadow-root-host"})
;; => "ac5ab914-7f93-427f-a0bf-f7e91098fd37"

which expects a return of "ac5ab914-7f93-427f-a0bf-f7e91098fd37" always and will therefore fail.

Options:

  1. Add a //:test-doc-blocks/skip before the code block to have test-doc-blocks skip the code block
  2. Remove the ;; => lines from the code example, test-doc-block will only expect the code to run without exception and not expect a specific return. If you feel it important the reader of the doc see a typical return, you could instead express that in a comment or descriptive text outside of the code block
  3. In some cases, consider rewriting the code sample, for example this:
    (e/query-shadow-root driver {:id "shadow-root-host"} {:css "#in-shadow"})
    ;; => "30fca382-6d4a-4f8a-9534-db76a1ed7cba"
    (e/get-element-text-el driver "30fca382-6d4a-4f8a-9534-db76a1ed7cba")
    ;; => "I'm in the shadow DOM"
    Could become:
    (->> (e/query-shadow-root driver {:id "shadow-root-host"} {:css "#in-shadow"})
         (e/get-element-text-el driver))
    ;; => "I'm in the shadow DOM"

I reviewed the user guide for what strategy I took for the element id returns, and it looks like I entirely avoided it in code blocks. It seems I spent a little bit of time upfront describing the difference between -el and non-el variants and didn't dwell on it at all later.

For me, it is a balance between "what does the reader need to see to understand this?" and "can I ensure that the code blocks should always work in the reader's REPL when they try them out?".

Anyhoo, great work, and I am happy to work out the above with you further.

@dgr
Copy link
Contributor Author

dgr commented Jul 14, 2024

OK, let me do one more commit with updates to doc strings and doc-test stuff.

Copy link
Collaborator

@lread lread left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting real close @dgr! Just a few minor things I noticed.

src/etaoin/api.clj Outdated Show resolved Hide resolved
src/etaoin/api.clj Outdated Show resolved Hide resolved
src/etaoin/api.clj Outdated Show resolved Hide resolved
src/etaoin/api.clj Show resolved Hide resolved
src/etaoin/api.clj Outdated Show resolved Hide resolved
src/etaoin/api.clj Show resolved Hide resolved
doc/01-user-guide.adoc Outdated Show resolved Hide resolved
doc/01-user-guide.adoc Outdated Show resolved Hide resolved
@dgr
Copy link
Contributor Author

dgr commented Jul 16, 2024

OK, try it again.

@lread
Copy link
Collaborator

lread commented Jul 16, 2024

Looks great @dgr, thanks!

@lread lread merged commit 22b225d into clj-commons:master Jul 16, 2024
53 checks passed
@lread lread mentioned this pull request Jul 17, 2024
@dgr dgr deleted the dgr-shadowdom branch July 17, 2024 15:34
lread added a commit that referenced this pull request Jul 28, 2024
…ridriver-logs

* origin/master:
  Ignore emacs backup files (#609)
  Add :fn/index as alias for :index in map syntax (#603) (#608)
  doc: thank Dave Roberts for contribution [skip ci] (#607)
  Add support for shadow DOM #604 (#606)
  doc: user-guide: add list of :fn/* (#605)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants